boards/stm32f4discovery: default to stdio via CDC ACM #19259

benpicco · 2023-02-07T21:16:20Z

Contribution description

Looks like stm32f4discovery also has one of the old on-board ST-Links that don't implement a USB-UART interface (like on stm32f429i-disco).
Since wiring up a USB-UART interface manually is annoying and the board provides a micro-USB port, default to stdio_cdc_acm so the user only needs an extra USB cable to access the shell.

Testing procedure

Connect a micro-USB cable to he port of the stm32f4discovery.
It should be recognized as a CDC ACM interface and make term should automatically connect to it.

I don't have this board.

Issues/PRs references

#19248 (comment)

riot-ci · 2023-02-07T21:50:01Z

Murdock results

✔️ PASSED

1cd2c16 tests/xtimer_now{32, 64}: blacklist stm32f4discovery

Success	Failures	Total	Runtime
555	0	555	02m:53s

Artifacts

Documentation preview

boards/stm32f4discovery/doc.txt

gschorcht · 2023-02-07T21:52:53Z

boards/stm32f4discovery/stm32f4discovery.config

@@ -0,0 +1,3 @@
+CONFIG_MODULE_USBUS=y


IMHO, this will cause a problem when compiling tinyUSB apps.

Hm I just copied that from stm32f429i-disco, not sure how this could be done better in Kconfig.

TEST_KCONFIG=1 BOARD=stm32f4discovery make -C tests/pkg_tinyusb_cdc_msc info-modules

=== [ATTENTION] Testing Kconfig dependency modelling === [genconfig.py]:ERROR-=> The choice MODULE_STDIO_CDC_ACM (defined at /home/gs/src/RIOT-Xtensa-ESP.esp-idf-4.4/sys/usb/usbus/cdc/acm/Kconfig:57) was selected but was not set.

I have also problems with stm32f429i-disco. Maybe they are not compiled with TEST_KCONFIG=1 in CI.

That's the reason why enabling the tinyusb_device feature for boards with a single USB interface doesn't work and why PR #18998 which tried to enable tinyusb_device for all such boards hangs since 3 months.

Probably the problem in Kconfig could be solved for the moment as in boards/esp32s3-pros3/Kconfig by overriding the default settings in STDIO_IMPLEMENTATION.

choice STDIO_IMPLEMENTATION bool "STDIO implementation" depends on TEST_KCONFIG default MODULE_STDIO_CDC_ACM if MODULE_USBUS default MODULE_STDIO_TINYUSB_CDC_ACM if MODULE_TINYUSB_DEVICE endchoice

gschorcht · 2023-02-08T00:46:54Z

bors try

gschorcht

Lets try it with that config.

bors · 2023-02-08T04:25:54Z

try

Build succeeded:

aabadie · 2023-02-08T06:34:04Z

I used to have this board and iirc the UART was available via stlink. I think some people in Berlin or Hamburg have it for testing.
Also note that there's a new "disc1" version of that board, so the "discovery" is no longer available I think

gschorcht · 2023-02-08T06:40:55Z

I used to have this board and iirc the UART was available via stlink. I think some people in Berlin or Hamburg have it for testing.
Also note that there's a new "disc1" version of that board, so the "discovery" is no longer available I think

I see. So we should have the same hack as with stm32f429i-disco and stm32f429-disc1.

benpicco · 2023-02-08T11:44:10Z

Ok so the new version is called stm32f407g-disc1.
Should we rename this board and add a stm32f4discovery that then includes the new board but with the added CDC ACM STDIO module?

I reckon the UART to the ST-Link on the new board is already configured correctly if you have it.

gschorcht · 2023-02-09T06:37:52Z

Ok so the new version is called stm32f407g-disc1.
Should we rename this board and add a stm32f4discovery that then includes the new board but with the added CDC ACM STDIO module?

I think we can stop the discussion about renaming. While writing this comment, I realized that my board is already that new version. While looking for the USART pins used for ST-LINK connection I had to realize that there is still no connection.

For comparison the ST-LINK connection of STM32F429i-DISC1 looks like.

gschorcht · 2023-02-09T06:42:44Z

From the current UM for STM32F407G-DISC1:

gschorcht · 2023-02-09T09:51:44Z

@benpicco The following changes solve the default selection problem of the STDIO backend in Kconfig. The boards/stm32f4discovery/stm32f4discovery.config is no longer used so that the STDIO backend selection also works with TEST_KCONFIG=1 for tinyusb which doesn't if the configuration is hard-coded by the boards/stm32f4discovery/stm32f4discovery.config file. I have tested it for non-USB application as well as for a tinyUSB application and an USBUS application with a temporary tests/usbus_hid/app.config.test.

diff --git a/boards/stm32f4discovery/Kconfig b/boards/stm32f4discovery/Kconfig
index 9a006f678d..39e929f976 100644
--- a/boards/stm32f4discovery/Kconfig
+++ b/boards/stm32f4discovery/Kconfig
@@ -35,6 +35,14 @@ config BOARD_STM32F4DISCOVERY
 
     select HAVE_SAUL_GPIO
 
+    select MODULE_USBUS_CDC_ACM if TEST_KCONFIG && !PACKAGE_TINYUSB
+    select PACKAGE_TINYUSB if TEST_KCONFIG && !MODULE_USBUS
+
+choice STDIO_IMPLEMENTATION
+    default MODULE_STDIO_CDC_ACM if MODULE_USBUS
+    default MODULE_STDIO_TINYUSB_CDC_ACM if PACKAGE_TINYUSB
+endchoice
+
 source "$(RIOTBOARD)/common/stm32/Kconfig"
 
 config ERROR_MODULES_CONFLICT
diff --git a/boards/stm32f4discovery/stm32f4discovery.config b/boards/stm32f4discovery/stm32f4discovery.config
deleted file mode 100644
index 7699f35a5c..0000000000
--- a/boards/stm32f4discovery/stm32f4discovery.config
+++ /dev/null
@@ -1,3 +0,0 @@
-CONFIG_MODULE_USBUS=y
-CONFIG_MODULE_USBUS_CDC_ACM=y
-CONFIG_MODULE_STDIO_CDC_ACM=y

gschorcht · 2023-02-09T16:30:48Z

From the current UM for STM32F407G-DISC1:

This is quite frustrating. What sense does the VCOM make if the pins are not hard-wired?

gschorcht · 2023-02-09T16:31:16Z

Please squash.

benpicco · 2023-02-09T16:51:58Z

Uh but looks like Kconfig is not happy.

benpicco · 2023-02-09T17:01:01Z

I went back to the original solution as at least that one is accepted by CI.

gschorcht · 2023-02-09T17:16:41Z

I went back to the original solution as at least that one is accepted by CI.

But this is a wrong picture. Once you try to compile any tinyUSB app with TEST_KCONFIG=1, you will run into a problem. I tested selected modules with tests/shell, tests/pkg_tinyusb_cdc_acm and tests/usbus_hid and a temporary tests/usbus_hid/app.config.text without problems. Let me check the examples/hello_world problem.

gschorcht · 2023-02-09T17:20:32Z

The problem is that `boards/stm32f4discovery/stm32f4discovery.config hard codes the USBUS CDC ACM interface. IMHO, the problem is of course the wrong modelling approach of STDIO backends in Kconfig and I really hope we can solve it.

gschorcht

I chaned to Request changes to avoid an accidental merge as long as I investigate the Kconfig problem.

gschorcht · 2023-02-09T17:51:00Z

Could you try the following please?

diff --git a/boards/stm32f4discovery/Kconfig b/boards/stm32f4discovery/Kconfig
index 9a006f678d..e405c8b46f 100644
--- a/boards/stm32f4discovery/Kconfig
+++ b/boards/stm32f4discovery/Kconfig
@@ -35,6 +35,14 @@ config BOARD_STM32F4DISCOVERY
 
     select HAVE_SAUL_GPIO
 
+    select MODULE_USBUS if TEST_KCONFIG && !PACKAGE_TINYUSB
+    select MODULE_USBUS_CDC_ACM if TEST_KCONFIG && !PACKAGE_TINYUSB
+
+choice STDIO_IMPLEMENTATION
+    default MODULE_STDIO_CDC_ACM if MODULE_USBUS
+    default MODULE_STDIO_TINYUSB_CDC_ACM if PACKAGE_TINYUSB
+endchoice
+
 source "$(RIOTBOARD)/common/stm32/Kconfig"
 
 config ERROR_MODULES_CONFLICT
diff --git a/boards/stm32f4discovery/stm32f4discovery.config b/boards/stm32f4discovery/stm32f4discovery.config
deleted file mode 100644
index 7699f35a5c..0000000000
--- a/boards/stm32f4discovery/stm32f4discovery.config
+++ /dev/null
@@ -1,3 +0,0 @@
-CONFIG_MODULE_USBUS=y
-CONFIG_MODULE_USBUS_CDC_ACM=y
-CONFIG_MODULE_STDIO_CDC_ACM=y

Using MODULE_STDIO_TINYUSB_CDC_ACM when PACKAGE_TINYUSB is enabled for some reason is sufficient to select all tinyusb_* modules that are needed, since the dependency logic for tinyUSB is modeled as in makefiles, i.e. if MODULE_STDIO_TINYUSB_CDC_ACM is used, all needed modules will be pulled enabled.

I wished it would be the same for MODULE_STDIO_CDC_ACM but the logic is inverse. The user has first to enable MODULE_USUSB, then MODULE_USBUS_CDC_ACM becomes visible and can be enabled and afterwards MODULE_STDIO_CDC_ACM becomes visible and can be enabled. Thats why the USBUS module have to be selected explicitly in board description. This approach make sense for optional modules but doesn't make sense in case of STDIO default backend selection. I hope this will change.

gschorcht · 2023-02-09T17:53:13Z

With these changes, the board should use MODULE_STDIO_CDC_ACM by default if tinyUSB is not used. In fact the same as using boards/stm32f4discovery/stm32f4discovery.config but it allows also to use tinyUSB.

benpicco · 2023-02-09T17:57:22Z

Thank you, CI seems to like this one.

gschorcht · 2023-02-10T05:49:56Z

Please squash.

benpicco · 2023-02-10T10:12:37Z

bors merge

bors · 2023-02-10T11:05:25Z

Build succeeded:

github-actions bot added Area: boards Area: Board ports Area: doc Area: Documentation labels Feb 7, 2023

benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 7, 2023

benpicco requested a review from gschorcht February 7, 2023 21:17

gschorcht requested changes Feb 7, 2023

View reviewed changes

benpicco requested review from smlng, leandrolanzieri, aabadie, MichelRottleuthner and fjmolinas as code owners February 7, 2023 21:54

github-actions bot added the Area: tests Area: tests and testing framework label Feb 7, 2023

benpicco force-pushed the boards/stm32f4discovery-cdc_acm branch 2 times, most recently from 5448d0f to a1ec273 Compare February 7, 2023 22:24

benpicco requested review from jia200x and MrKevinWeiss as code owners February 7, 2023 22:24

github-actions bot added the Area: Kconfig Area: Kconfig integration label Feb 7, 2023

benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 7, 2023

bors bot added a commit that referenced this pull request Feb 8, 2023

Try #19259:

4894107

gschorcht approved these changes Feb 8, 2023

View reviewed changes

benpicco force-pushed the boards/stm32f4discovery-cdc_acm branch from 31fa60c to b31ec28 Compare February 9, 2023 16:50

benpicco force-pushed the boards/stm32f4discovery-cdc_acm branch from b31ec28 to c57ff38 Compare February 9, 2023 17:00

gschorcht requested changes Feb 9, 2023

View reviewed changes

gschorcht approved these changes Feb 10, 2023

View reviewed changes

benpicco added 2 commits February 10, 2023 10:12

boards/stm32f4discovery: default to stdio via CDC ACM

3e1f5ed

tests/xtimer_now{32, 64}: blacklist stm32f4discovery

1cd2c16

benpicco force-pushed the boards/stm32f4discovery-cdc_acm branch from e42c85d to 1cd2c16 Compare February 10, 2023 09:12

bors bot merged commit d145a87 into RIOT-OS:master Feb 10, 2023

benpicco deleted the boards/stm32f4discovery-cdc_acm branch February 10, 2023 11:17

aabadie mentioned this pull request Mar 10, 2023

boards/stm32f4discovery: use default port to access stdio via cdc acm #19376

Merged

MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

boards/stm32f4discovery: default to stdio via CDC ACM #19259

boards/stm32f4discovery: default to stdio via CDC ACM #19259

benpicco commented Feb 7, 2023 •

edited

Loading

riot-ci commented Feb 7, 2023 •

edited

Loading

gschorcht Feb 7, 2023

benpicco Feb 7, 2023

gschorcht Feb 7, 2023

gschorcht Feb 7, 2023

gschorcht Feb 7, 2023

gschorcht Feb 8, 2023

gschorcht Feb 8, 2023

gschorcht commented Feb 8, 2023

gschorcht left a comment

bors bot commented Feb 8, 2023

aabadie commented Feb 8, 2023

gschorcht commented Feb 8, 2023

benpicco commented Feb 8, 2023

gschorcht commented Feb 9, 2023 •

edited

Loading

gschorcht commented Feb 9, 2023 •

edited

Loading

gschorcht commented Feb 9, 2023 •

edited

Loading

gschorcht commented Feb 9, 2023 •

edited

Loading

gschorcht commented Feb 9, 2023

benpicco commented Feb 9, 2023

benpicco commented Feb 9, 2023

gschorcht commented Feb 9, 2023 •

edited

Loading

gschorcht commented Feb 9, 2023

gschorcht left a comment

gschorcht commented Feb 9, 2023

gschorcht commented Feb 9, 2023

benpicco commented Feb 9, 2023

gschorcht commented Feb 10, 2023

benpicco commented Feb 10, 2023

bors bot commented Feb 10, 2023

boards/stm32f4discovery: default to stdio via CDC ACM #19259

boards/stm32f4discovery: default to stdio via CDC ACM #19259

Conversation

benpicco commented Feb 7, 2023 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

riot-ci commented Feb 7, 2023 • edited Loading

Murdock results

Artifacts

gschorcht Feb 7, 2023

Choose a reason for hiding this comment

benpicco Feb 7, 2023

Choose a reason for hiding this comment

gschorcht Feb 7, 2023

Choose a reason for hiding this comment

gschorcht Feb 7, 2023

Choose a reason for hiding this comment

gschorcht Feb 7, 2023

Choose a reason for hiding this comment

gschorcht Feb 8, 2023

Choose a reason for hiding this comment

gschorcht Feb 8, 2023

Choose a reason for hiding this comment

gschorcht commented Feb 8, 2023

gschorcht left a comment

Choose a reason for hiding this comment

bors bot commented Feb 8, 2023

try

aabadie commented Feb 8, 2023

gschorcht commented Feb 8, 2023

benpicco commented Feb 8, 2023

gschorcht commented Feb 9, 2023 • edited Loading

gschorcht commented Feb 9, 2023 • edited Loading

gschorcht commented Feb 9, 2023 • edited Loading

gschorcht commented Feb 9, 2023 • edited Loading

gschorcht commented Feb 9, 2023

benpicco commented Feb 9, 2023

benpicco commented Feb 9, 2023

gschorcht commented Feb 9, 2023 • edited Loading

gschorcht commented Feb 9, 2023

gschorcht left a comment

Choose a reason for hiding this comment

gschorcht commented Feb 9, 2023

gschorcht commented Feb 9, 2023

benpicco commented Feb 9, 2023

gschorcht commented Feb 10, 2023

benpicco commented Feb 10, 2023

bors bot commented Feb 10, 2023

benpicco commented Feb 7, 2023 •

edited

Loading

riot-ci commented Feb 7, 2023 •

edited

Loading

gschorcht commented Feb 9, 2023 •

edited

Loading

gschorcht commented Feb 9, 2023 •

edited

Loading

gschorcht commented Feb 9, 2023 •

edited

Loading

gschorcht commented Feb 9, 2023 •

edited

Loading

gschorcht commented Feb 9, 2023 •

edited

Loading